Skip to content

add task solution#5448

Open
diiyavol wants to merge 9 commits into
mate-academy:masterfrom
diiyavol:develop
Open

add task solution#5448
diiyavol wants to merge 9 commits into
mate-academy:masterfrom
diiyavol:develop

Conversation

@diiyavol

@diiyavol diiyavol commented Jun 25, 2026

Copy link
Copy Markdown

DEMO LINK
TEST REPORT LINK

  • Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout
  • Each day has no modifiers, only class (eg. calendar__day)
  • All Typical Mistakes from BEM lesson theory are checked.
  • Code follows all the Code Style Rules ❗️

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because several core requirements from the task and checklist are not yet met, and they affect both correctness and layout. First, styles are implemented in src/styles/index.scss and your HTML links that file, but the task explicitly requires using src/styles/main.scss as the main stylesheet, so you need to move the styles into main.scss and update the <link> in src/index.html accordingly. Second, the calendar width and start-day offset are based on hardcoded values like max-width: 800px, width: 100px, gap: 1px, and padding-left: 10px, instead of using SCSS variables to clearly compute "7 columns + 10px paddings"; this violates the rule against repeating raw px values and makes the start-day and month-length modifiers less robust. Third, the hover animation currently only transitions transform 500ms, while the spec requires all hover changes (movement and background color) to be animated over 0.5s, so you should include background-color in the transition (e.g. transition: transform 0.5s, background-color 0.5s). Finally, the cursor is always pointer on .calendar__day, but the requirement says the cursor should become pointer on hover, so move cursor: pointer; into the :hover rule. Once you fix the stylesheet location, introduce and use SCSS variables for day size/border/gap/padding in all layout calculations, and update the hover behavior to animate both transform and background-color only on hover, your solution will align well with the task and can be approved.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/index.html
Comment on lines 13 to +15
/>
</head>
<body>
<h1>Calendar</h1>
<body class="main">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calendar width must be calculated as exactly 7 columns plus 10px paddings using SCSS variables, but here you hardcode max-width: 800px and don’t base it on the cell size, border, gap, and padding variables as required ("Don't use hardcoded px values if they are used several times" and "Use properly named variables to make all the calculations more clear").

Comment thread src/index.html
Comment on lines +22 to +28
<div class="calendar__day"></div>
<div class="calendar__day"></div>
<div class="calendar__day"></div>
<div class="calendar__day"></div>
<div class="calendar__day"></div>
<div class="calendar__day"></div>
<div class="calendar__day"></div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task states all hover changes (transform and background color) must be animated with duration 0.5s, but this transition only animates transform and not background-color, and uses 500ms without including background-color. Consider adding background-color to the transition and ensuring the duration matches 0.5s.

Comment thread src/styles/main.scss
Comment on lines 10 to +12
margin: 0;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the description, styles must be written in src/styles/main.scss instead of src/style.css, and your HTML should link that file. Right now you link styles/index.scss, so you need to move the styles into main.scss and update this href accordingly.

Comment thread src/styles/main.scss
@@ -1,3 +1,75 @@
* {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requirements say: "Write styles in src/styles/main.scss instead of src/style.css" and also "Use properly named variables to make all the calculations more clear". Here you’re using index.scss and not using variables for shared values (day size, border, gap, padding), so consider moving this file to main.scss and introducing SCSS variables for these repeated px values.

Comment thread src/styles/index.scss Outdated
Comment on lines +13 to +15
.calendar {
display: inline-flex;
max-width: 800px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calendar width must be limited to exactly 7 columns + 10px paddings, and the description asks not to hardcode px values used several times but to use variables for clearer calculations. max-width: 800px; is a magic number; instead, derive the width from the day size, border, gap, and padding using SCSS variables.

Comment thread src/styles/index.scss Outdated
Comment on lines +13 to +18
.calendar {
display: inline-flex;
max-width: 800px;
flex-wrap: wrap;
align-self: center;
gap: 1px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates the requirement "Don't use hardcoded px values if they are used several times" and "Use properly named variables to make all the calculations more clear". gap: 1px; is used here and combined in other calculations, so it should be represented via a SCSS variable rather than repeated raw values.

Comment thread src/styles/index.scss Outdated
Comment on lines +13 to +19
.calendar {
display: inline-flex;
max-width: 800px;
flex-wrap: wrap;
align-self: center;
gap: 1px;
padding-left: 10px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Padding is part of the "width = 7 columns + 10px paddings" requirement and is also a repeated px value. Instead of hardcoding 10px here, define a SCSS variable (e.g. $calendar-padding) and use it consistently when computing width and layout.

Comment thread src/styles/index.scss Outdated
Comment on lines +22 to +28
.calendar__day {
cursor: pointer;
width: 100px;
height: 100px;
background-color: #eee;
border: 1px solid black;
transition: transform 500ms;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hover behavior is required to animate all changes (movement and color) over 0.5s. Here transition: transform 500ms; only animates transform. Consider transitioning both transform and background-color (and using 0.5s as the duration) to fully satisfy the hover animation requirement.

Comment thread src/styles/index.scss Outdated
Comment on lines +22 to +23
.calendar__day {
cursor: pointer;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec states "On hovering over a cell: cursor should become pointer", meaning the cursor change should be part of the hover state. Currently cursor: pointer; is set on the base .calendar__day, so the cursor is always a pointer even when not hovering. Move the cursor declaration to the :hover rule to match the requirement.

Comment thread src/styles/main.scss Outdated
Comment on lines +61 to +64
@each $day, $index in $start-days {
.calendar--start--day--#{$day} {
.calendar__day:first-child {
margin-left: calc((100px + 1px) * #{$index});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This margin-left calculation uses hardcoded 100px and 1px, which are also used as width, height, and gap elsewhere. The description forbids repeating such hardcoded px values and asks for named variables. Introduce SCSS variables (e.g. $day-size, $day-border, $day-gap) and use them here so the start-day offset stays consistent with the actual cell dimensions.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there is still a critical requirement violation around the use of hardcoded pixel values in your SCSS layout math. The task explicitly says the calendar width must be “exactly 7 columns + 10px paddings” and that repeated px values must be expressed via variables, but you currently have $calendar__width: 800px;, height: 100px; on .calendar__day, and margin-left: calc((100px + 1px) * #{$index}); (see src/styles/main.scss) using raw literals instead of $day__width, $days__gap, and $paddings. This means changing the day size or gap variables will not correctly update the calendar width or start-day offset, which breaks the requirement that modifiers and sizing variables cleanly control the layout [CHECKLIST ITEM #1].

To fix this, derive $calendar__width from your size variables, for example using a formula based on 7 * $day__width, 6 * $days__gap, and $paddings, instead of a magic number. Also, replace height: 100px; with height: $day__width; (or a new $day__size variable) so each day is a true square driven by SCSS variables, and refactor the margin-left for start-day to use ($day__width + $days__gap) instead of 100px and 1px. Once these variable-based calculations are in place, your otherwise solid implementation will fully meet the task requirements and be much easier to maintain. You’ve done a good job on the core calendar behavior and hover effects—after these layout variable fixes, your solution should be ready to pass.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/styles/main.scss Outdated
@@ -0,0 +1,80 @@
$calendar__width: 800px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires limiting calendar width to "exactly 7 columns + 10px paddings" and avoiding repeated hardcoded px values. Here $calendar__width: 800px; is a magic number not derived from $day__width, $days__gap, and $paddings, which violates that requirement; consider computing this width from those variables instead.

Comment thread src/styles/main.scss Outdated

.calendar__day {
width: $day__width;
height: 100px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

height: 100px; repeats the day size as a literal while you already have $day__width. The description says not to use hardcoded px values when reused; consider using a variable (e.g. $day__size) for both width and height so each day is a 100px square driven by SCSS variables.

Comment thread src/styles/main.scss Outdated
@each $day, $index in $start-days {
.calendar--start--day--#{$day} {
.calendar__day:first-child {
margin-left: calc((100px + 1px) * #{$index});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This margin-left uses hardcoded 100px and 1px instead of $day__width and $days__gap. The requirements explicitly say to avoid hardcoded px values used several times and to use variables for all calculations (including the 7 columns + 10px paddings and start-day offsets). Refactor this expression to rely on your SCSS variables, not raw literals.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can keep progressing while you refine these last details. You did a strong job on the core calendar functionality: 31 days generated via Sass @for, modifiers for start-day and month-length that correctly shift and hide days, clean BEM structure, and hover effects that match the spec. The main layout issue is the width: instead of max-width: $calendar__count * ($day__width + $paddings);, derive a single $calendar__width like 7 * $day__width + 6 * $days__gap + 2 * $paddings and apply symmetric horizontal padding (e.g. padding: 0 $paddings;) so the calendar truly matches “7 columns + 10px paddings” [CHECKLIST ITEM #1]. Another improvement is to avoid a separate hardcoded $day__height: 100px; and tie the height directly to the width (for example height: $day__width; or use a shared $day__size variable), so changing one size variable keeps the cells perfectly square and removes repeated pixel literals. These changes will also help the visual tests align with the expected screenshots and keep your SCSS variables as the single source of truth for sizing. Overall, your structure and use of Sass are solid; with these tweaks to width and day sizing, your calendar will fully match the requirements and be a great reference implementation.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/index.html
<div class="calendar__day"></div>
<div class="calendar__day"></div>
<div class="calendar__day"></div>
<div class="calendar__day"></div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires the calendar width to be "exactly 7 columns + 10px paddings" and asks not to reuse hardcoded px values. max-width: $calendar__count * ($day__width + $paddings); adds $paddings per column instead of a single padding value for the whole block, so changing $day__width or $days__gap will not correctly keep "7 columns + 10px paddings". Consider computing width from 7 * $day__width, 6 * $days__gap, and a single horizontal padding value as per the description.

Comment thread src/index.html
Comment on lines +25 to +26
<div class="calendar__day"></div>
<div class="calendar__day"></div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With padding-left: $paddings; only, the calendar gets padding on one side, while the requirement talks about 10px paddings (plural). Together with the current max-width formula, this prevents the layout from truly reflecting "exactly 7 columns + 10px paddings". Consider applying padding symmetrically and updating the width calculation so that modifiers and size variables correctly control the layout as required by checklist item "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout".

Comment thread src/styles/main.scss Outdated

.calendar {
display: inline-flex;
max-width: $calendar__count * ($day__width + $paddings);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your max-width calculation max-width: $calendar__count * ($day__width + $paddings); does not match the requirement "limit its width to exactly 7 columns + 10px paddings" and the previous correction about deriving width from size variables. Padding is being added per column here instead of once for the container, and the 1px gap between cells is not included in the formula. Consider deriving a dedicated $calendar__width from the variables as 7 * $day__width + 6 * $days__gap + 2 * $paddings and using that, so changing day__width, days__gap, or paddings keeps the layout consistent.
This is related to the generalized requirement: "Derive $calendar__width from size variables instead of using a hardcoded 800px, so the calendar width matches ‘exactly 7 columns + 10px paddings’ and updates correctly when day size or gap variables change."

Comment thread src/styles/main.scss Outdated
Comment on lines +4 to +31
$day__height: 100px;
$days__gap: 1px;
$paddings: 10px;

* {
box-sizing: border-box;
}

body {
display: flex;
justify-content: center;
align-items: center;
height: $body__height;
margin: 0;
}

.calendar {
display: inline-flex;
max-width: $calendar__count * ($day__width + $paddings);
flex-wrap: wrap;
align-self: center;
gap: $days__gap;
padding-left: $paddings;
}

.calendar__day {
width: $day__width;
height: $day__height;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You introduced $day__height: 100px; as a separate literal, but the previous correction explicitly required replacing height: 100px; with a variable-based expression tied to the day size (e.g. using $day__width or a dedicated $day__size variable). Right now, changing $day__width will not automatically keep the cells square. To meet the requirement "Don’t use hardcoded px values if they are used several times" and the previous review note, consider either removing $day__height and using height: $day__width; or defining a single $day__size variable used for both width and height.
This still violates the generalized correction: "Replace height: 100px; on .calendar__day with a variable-based expression (e.g., using $day__width or a $day__size variable) so each day is a true square controlled by SCSS variables."

@Denys-Kravchuk9988 Denys-Kravchuk9988 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEMO LINK doesn't work. Please, check the deployment.
Try to pass all tests

Image

@brespect brespect left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress, but let's pass all tests before requesting the review

@diiyavol diiyavol requested a review from brespect June 26, 2026 17:42

@etojeDenys etojeDenys left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants